Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lint] replaces black formatting with ruff #14132

Merged
merged 13 commits into from
Jan 11, 2024

Conversation

iris-garden
Copy link
Collaborator

@iris-garden iris-garden commented Jan 9, 2024

ruff is faster than black when formatting Python files, so this change replaces the formatting checks for our files with calls to ruff instead of black, and runs ruff format on all files that would cause those checks to fail.

@iris-garden
Copy link
Collaborator Author

oh wait, okay, i'm gonna see if there's an equivalent to skip-string-normalization for ruff, since i think that might be literally all of these automatic changes haha

batch/batch/driver/main.py Fixed Show fixed Hide fixed
batch/batch/cloud/azure/worker/worker_api.py Fixed Show fixed Hide fixed
ci/ci/ci.py Fixed Show fixed Hide fixed
ci/create_database.py Fixed Show fixed Hide fixed
batch/batch/driver/job.py Fixed Show fixed Hide fixed
ci/create_database.py Fixed Show fixed Hide fixed
@iris-garden iris-garden force-pushed the lint/ruff-replace-black-format branch from 97b15b3 to 44d9c7e Compare January 9, 2024 19:15
@iris-garden
Copy link
Collaborator Author

iris-garden commented Jan 9, 2024

okay, enabled the quote-style = "preserve" flag, now this should be actually ready for a look! (there are still changes to quotes around some docstrings, but there's not a way to disable this for ruff currently)

@daniel-goldstein
Copy link
Contributor

daniel-goldstein commented Jan 9, 2024

Interesting, PEP 8 does have an opinion about triple-quoted strings. FWIW, I'm happy to go stricter in the future and allow string normalization and all that (assuming other folks are on board), but I appreciate you aligning the configs for this PR!

Copy link
Contributor

@daniel-goldstein daniel-goldstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! One last request and I'll approve: could you remove black from the dev requirements and replace the pre-commit hook?

f' dataset {name}.\n'
f'Available platforms: {clouds}.')
raise ValueError(
f'Cloud platform {repr(cloud)} not available for' f' dataset {name}.\n' f'Available platforms: {clouds}.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is a little unfortunate. Let's combine them into one f-string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@iris-garden
Copy link
Collaborator Author

iris-garden commented Jan 9, 2024

fixed up the dev requirements, and updated the pre-commit hook, and in the process realized i had only run ruff format on files that are checked by make check-all, so also ran it at the root of the repo, as the hook will run there

EDIT: i misunderstood how the pre-commit hooks work, and apparently it only runs them on the files you have changed, but i think it's still worth it to format across the repo, not just in the places we check with make check-all; and also, i personally was a grump and never used the pre-commit hooks, but now that i've had to test them out for this, i have seen the light ⚡

also i personally would love if our quotes were consistent as well! i'll ask on zulip how ppl feel about that

devbin/rotate_keys.py Dismissed Show dismissed Hide dismissed
@danking danking merged commit fa2ef0f into hail-is:main Jan 11, 2024
8 checks passed
danking pushed a commit that referenced this pull request Jan 16, 2024
chrisvittal pushed a commit to chrisvittal/hail that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants